[feature] Inventory inaccessible handling #236
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…Accessible) with rules/UI safety
…r blocks, and UI icon/tooltip
… repo null; remove flaky target-access test
… (Windows ACL + POSIX chmod), with cleanup
paul-fresquet
changed the title
test: add tests
test: add tests
test: add tests
Copilot finished reviewing on behalf of
paul-fresquet
November 15, 2025 13:45
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive handling for inaccessible files and directories during inventory scanning. When the application encounters permission issues (UnauthorizedAccessException, DirectoryNotFoundException, IOException), it now continues scanning while marking affected items as inaccessible rather than stopping the entire inventory process.
Key changes:
- Added
IsAccessibleproperty toFileSystemDescriptionmodel to track accessibility status - Implemented robust error handling in
InventoryBuilderwith try/catch blocks around file system operations - Created condition matcher abstraction to support synchronization rules
- Added UI indicators (warning icons and tooltips) for inaccessible items
- Implemented validators to block synchronization actions on inaccessible files
- Added comprehensive unit and integration tests with OS-specific permission handling
Reviewed Changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| FileSystemDescription.cs | Added IsAccessible boolean property with default value of true |
| InventoryBuilder.cs | Refactored to use EnumerateFiles/Directories, added try/catch blocks for access exceptions, mark inaccessible items |
| FileSystemInspector.cs | New abstraction for file system checks (hidden, system, reparse point, etc.) |
| ContentIdentity.cs | Added access issue tracking with AccessIssueInventoryParts collection and HasAccessIssue computed property |
| AtomicActionConsistencyChecker.cs | Added validation to block sync actions when source/target is inaccessible |
| ContentIdentityViewModel.cs | Added HasAccessIssue property and UI logic to show warning icons for inaccessible items |
| ComparisonItemViewModel.cs | Changed collections from HashSet to List for sorting, added sorting by inventory part codes |
| InventoryComparer.cs | Added PropagateAccessIssuesFromAncestors to mark items under inaccessible directories |
| SynchronizationRuleMatcher.cs | Refactored to use condition matcher factory pattern |
| ConditionMatchers/* | New matcher classes for content, size, date, presence, and name conditions |
| ContentIdentityExtractor.cs | Extracted content identity retrieval logic from matcher |
| Resources files | Added localization strings for access-related errors and UI messages |
| Test files | Comprehensive unit and integration tests with Windows ACL and POSIX permission handling |
| docs/testing-permissions-and-symlinks.md | Detailed documentation for testing permission scenarios |
| CI pipeline | Modified to handle integration tests differently on macOS (no coverage, added diagnostics) |
Files not reviewed (1)
- src/ByteSync.Client/Assets/Resources/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
src/ByteSync.Client/ViewModels/Sessions/Comparisons/Results/ContentIdentityViewModel.cs:207
- Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ByteSync.Client/Models/Comparisons/Result/ContentIdentity.cs
Outdated
Show resolved
Hide resolved
src/ByteSync.Client/Services/Comparisons/ContentIdentityExtractor.cs
Outdated
Show resolved
Hide resolved
tests/ByteSync.Client.UnitTests/Models/Comparisons/ContentIdentityAccessTests.cs
Outdated
Show resolved
Hide resolved
src/ByteSync.Client/Models/Comparisons/Result/ContentIdentity.cs
Outdated
Show resolved
Hide resolved
tests/ByteSync.Client.UnitTests/Services/Inventories/InventoryBuilderInspectorTests.cs
Outdated
Show resolved
Hide resolved
tests/ByteSync.Client.UnitTests/Services/Inventories/InventoryBuilderPublicTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
…tor.cs Co-authored-by: Copilot <[email protected]>
…andling' into feature/inventory-inaccessible-handling
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Inventory can hit access errors (e.g.
UnauthorizedAccessException, protected system folders, POSIX permissions). Previously this could stop the inventory and/or allow rules to propose actions to/from locations that were not actually accessible.What this PR does
IsAccessible(on all FileSystemDescription types).Technical details (summary)
bool IsAccessible { get; set; } = true;onFileSystemDescription.Get*()withEnumerate*()+try/catchonUnauthorizedAccessException,DirectoryNotFoundException,IOException.IsAccessible=false, not registered for analysis; identified volume not increased.ExistsOnconsiders presence even if inaccessible (prevents false "Create" proposals).AtomicActionConsistencyCheckerrejects when source or any target has aFileDescriptionwithIsAccessible=false.ContentIdentity: new computed propertyHasAccessIssue.ContentIdentityViewModel/View: warning icon + access-specific tooltip.Tests
ExistsOnreturns true (presence) while analysis is excluded.finally.chmod 000on the target file; same validation; permissions restored (0644) infinally.User impact
Compatibility
IsAccessible) default to accessible.Notes
Checklist